[PM-41690] fix: CXF import crash on negative timestamps - #1362
Conversation
Some credential managers (e.g., Google Password Manager) export creationAt/modifiedAt as the Windows FILETIME epoch (-11644473600) when no real date exists. The credential-exchange-format crate deserializes these fields as u64 and rejects negative values. Sanitize the JSON payload before deserialization by clamping negative timestamp values to 0.
|
|
|
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
harr1424
left a comment
harr1424
left a comment
There was a problem hiding this comment.
@crosenth This PR and the research you invested in it is greatly appreciated. I do disagree with the fundamental approach taken here, which diverges from v1.0 specifications:
creationAt
This OPTIONAL member contains the UNIX timestamp in seconds at which this Collection was originally created. If this member is not set, but the importing provider requires this member in their proprietary data model, the importer SHOULD use the current timestamp at the time the provider encounters this Collection.
modifiedAt
This OPTIONAL member contains the UNIX timestamp in seconds of the last modification brought to this Collection. If this member is not set, but the importing provider requires this member in their proprietary data model, the importer SHOULD use the current timestamp at the time the provider encounters this Collection.
The above also applies to these values nested in the items array.
Additionally, the fix has only been applied to negative values in the items array and not collection which can also introduce the negative values and cause a crash. Please see my comment in crates/bitwarden-exporters/src/cxf/tests/negative_timestamp_test.rs for a POC.
I've suggested code changes to improve the efficiency of the timestamp adjustment and ensure it results in the current timestamp.
I've also suggested some additional test coverage.
Co-authored-by: John Harrington <84741727+harr1424@users.noreply.github.com>
- Assert null instead of 0 for clamped timestamps - Add test verifying unmodified input returns borrowed reference
- Test negative timestamps in collections array - Test negative timestamps in nested subCollections - Test that clamped timestamps fall back to approximately current time
harr1424
left a comment
harr1424
left a comment
There was a problem hiding this comment.
I've left a suggestion to update a doc comment to describe the new behavior, please also update the PR description as it still describes the old behavior (assigning 0 instead of null).
Please also be sure to run the checks described here as your PR won't be able to merge until these pass.
Lastly I left a comment in the new test file asking if there was any reason not to add a test ensuring allocation is avoided when the payload doesn't need to be modified. If you'd prefer not to add this, it's probably OK, it would be nice to document your reasoning, though.
Co-authored-by: John Harrington <84741727+harr1424@users.noreply.github.com>
Use matches!(Cow::Borrowed(_)) to explicitly verify no allocation occurs, rather than just checking content equality.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1362 +/- ##
=======================================
Coverage 86.41% 86.41%
=======================================
Files 501 501
Lines 74235 74278 +43
=======================================
+ Hits 64149 64190 +41
- Misses 10086 10088 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@crosenth please be sure to run the lint described here in order to format these changes against standards, right now the required CI check "Lint / Check Style (pull_request)" is failing. You can run |
|
@crosenth we are waiting on a second code-review from a colleague on my team. Everything looks good on my end and I look forward to merging your changes as soon as they are reviewed by a second pair of eyes. @adudek-bw can you please give this a review? |
Thanks @harr1424 really enjoyed iterating on this with you |
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
…XF import crash on negative timestamps (bitwarden/sdk-internal#1362)
5 participants
Some credential managers (e.g., Google Password Manager) export creationAt/modifiedAt as the Windows FILETIME epoch (-11644473600) when no real date exists. The credential-exchange-format crate deserializes these fields as u64 and rejects negative values.
Sanitize the JSON payload before deserialization by clamping negative timestamp values to
null.🎟️ Tracking
bitwarden/android#7140
bitwarden/android#7215
bitwarden/android#7216
https://bitwarden.atlassian.net/browse/PM-40542
📔 Objective
Summary
creationAt/modifiedAtvalues in CXF payloadsbefore deserialization, clamping them to
nullcredentials have no real creation/modification date (exported as the
Windows FILETIME epoch: -11644473600)
Root cause
The
credential-exchange-formatcrate defines these fields asOption<u64>, which cannot represent negative values. The fixpre-processes the JSON in
parse_cxf()before the typeddeserialization step.
Test plan
sanitize_timestamps()covering negative,valid, and mixed timestamp values
parse_cxf()succeeds withnegative timestamps in the payload